Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

model: Use SingleLineString for all appropriate Strings #453

Merged
merged 1 commit into from
Oct 24, 2019
Merged

model: Use SingleLineString for all appropriate Strings #453

merged 1 commit into from
Oct 24, 2019

Conversation

tjkirch
Copy link
Contributor

@tjkirch tjkirch commented Oct 23, 2019

Don't want anyone inserting extra lines into our config files.

The String types that were not changed:

  • retry_commands because it's conceivable to want a multi-line command; these are read-only so not relevant to API calls right now.
  • The keys of Services and ConfigurationFiles maps, because they're handled dynamically in code and would require other changes I haven't figured out yet; these are read-only so not relevant to API calls right now.

Testing done:

Requests with newlines are now rejected in all these places; good requests are still OK.

bash-5.0# apiclient -u /settings -m PATCH -d '{"hostname": "hi\nthere"}'
Json deserialize error: Can't create SingleLineString with line terminator '2' at line 1 column 25

bash-5.0# apiclient -u /settings -m PATCH -d '{"hostname": "hi there"}' -v
204 No Content

All our templated files still show up OK, all units run OK, node joins cluster.

=== /etc/hostname ===
localhost

=== /etc/kubernetes/kubelet/env ===
NODE_IP=192.168.101.171
NODE_LABELS=
NODE_TAINTS=
POD_INFRA_CONTAINER_IMAGE=602401143452.dkr.ecr.us-west-2.amazonaws.com/eks/pause-amd64:3.1

=== /etc/kubernetes/kubelet/config ===
---
kind: KubeletConfiguration
...SNIP...
clusterDNS:
- 10.100.0.10
...SNIP...
MaxPods: 29

=== /etc/kubernetes/kubelet/kubeconfig ===
---
apiVersion: v1
kind: Config
clusters:
- cluster:
    certificate-authority: "/etc/kubernetes/pki/ca.crt"
    server: "https://16D8E306F5C75C5BF9C7FDA2A4DFAC90.yl4.us-west-2.eks.amazonaws.com"
    ...SNIP...

=== /etc/kubernetes/pki/ca.crt ===
-----BEGIN CERTIFICATE-----
MII ...SNIP... AA=
-----END CERTIFICATE-----

=== /etc/containerd/config.toml ===
...SNIP...
sandbox_image = "602401143452.dkr.ecr.us-west-2.amazonaws.com/eks/pause-amd64:3.1"
...SNIP...

=== /etc/updog.toml ===
...SNIP...
seed = 572

=== /etc/chrony.conf ===
pool 169.254.169.123 iburst

pool 2.amazon.pool.ntp.org iburst
...SNIP...

@iliana
Copy link
Contributor

iliana commented Oct 23, 2019

Does this require a migration?

@tjkirch
Copy link
Contributor Author

tjkirch commented Oct 24, 2019

Does this require a migration?

The data would be stored the same way, so... not according to the traditional definition. What would happen is that the API would refuse to load invalid data you had stored previously, after the upgrade. We could write a migration that truncates invalid data rather than failing, but I highly doubt anyone inserted invalid data that they care about, since it wouldn't work in the tools loading the config files.

iliana
iliana approved these changes Oct 24, 2019
View reviewed changes
Copy link
Contributor

@iliana iliana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would happen is that the API would refuse to load invalid data you had stored previously, after the upgrade.

How does the system behave in this state? Does it replace it with None and carry on?

I agree that we're still in the "this is unlikely input" stage but I want to make sure we're answering these questions every time we change settings codepaths :)

@tjkirch
Copy link
Contributor Author

tjkirch commented Oct 24, 2019

How does the system behave in this state? Does it replace it with None and carry on?

No, it'd be noisy and fail most requests related to settings; in its view, the data store would be essentially corrupted.

@tjkirch tjkirch merged commit 39a0660 into develop Oct 24, 2019
@tjkirch tjkirch deleted the model-strings branch October 24, 2019 22:56
@iliana iliana added this to the v0.2.0 milestone Nov 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iliana iliana iliana approved these changes

@bcressey bcressey bcressey approved these changes

@jamieand jamieand Awaiting requested review from jamieand

@zmrow zmrow Awaiting requested review from zmrow

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.2.0
Development

Successfully merging this pull request may close these issues.
3 participants
@tjkirch @iliana @bcressey